-
Notifications
You must be signed in to change notification settings - Fork 161
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix subtle bug when deserializing property without value #1308
base: main
Are you sure you want to change the base?
Fix subtle bug when deserializing property without value #1308
Conversation
5e18dff
to
3337a09
Compare
@@ -489,8 +489,14 @@ public virtual void ApplyStructuralProperties(object resource, ODataResourceWrap | |||
throw new ArgumentNullException(nameof(resourceWrapper)); | |||
} | |||
|
|||
foreach (ODataProperty property in resourceWrapper.Resource.Properties) | |||
foreach (ODataPropertyInfo propertyInfo in resourceWrapper.Resource.Properties) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why don't use 'OfType', so we don't need line 494 to 499.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was deliberate. I believe OfType
would be more expensive
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xuzhg Here's the code for a simple benchmark to compare the two:
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Diagnosers;
using System.Collections.Generic;
[MemoryDiagnoser]
public class Benchmarks
{
private IEnumerable<ODataPropertyInfo> properties;
public Benchmarks()
{
List<ODataPropertyInfo> properties = new()
{
new ODataProperty { Name = "A", Value = 1 },
new ODataProperty { Name = "A", Value = 1 },
new ODataProperty { Name = "A", Value = 1 },
new ODataProperty { Name = "A", Value = 1 },
new ODataProperty { Name = "A", Value = 1 },
new ODataProperty { Name = "A", Value = 1 },
new ODataProperty { Name = "A", Value = 1 },
new ODataProperty { Name = "A", Value = 1 },
new ODataPropertyInfo { Name = "A" },
new ODataProperty { Name = "A", Value = 1 },
new ODataProperty { Name = "A", Value = 1 },
new ODataProperty { Name = "A", Value = 1 },
new ODataProperty { Name = "A", Value = 1 },
new ODataPropertyInfo { Name = "A" },
new ODataProperty { Name = "A", Value = 1 },
new ODataProperty { Name = "A", Value = 1 },
};
this.properties = properties;
}
[Benchmark]
public int CountPropertyValues_WithNestedCondition()
{
int count = 0;
foreach (ODataPropertyInfo propertyInfo in properties)
{
if (propertyInfo is ODataProperty property) {
count += property.Value;
}
}
return count;
}
[Benchmark]
public int CountPropertyValues_WithOfType()
{
int count = 0;
foreach (ODataProperty property in properties.OfType<ODataProperty>())
{
count += property.Value;
}
return count;
}
}
public class ODataPropertyInfo
{
public string Name { get; set; }
}
public class ODataProperty : ODataPropertyInfo
{
public int Value { get; set; }
}
I think that LINQ overheads are responsible for the difference
@@ -200,6 +200,10 @@ private static void ReadODataItem(ODataReader reader, Stack<ODataItemWrapper> it | |||
resourceSetParentWrapper.Items.Add(new ODataPrimitiveWrapper((ODataPrimitiveValue)reader.Item)); | |||
break; | |||
|
|||
case ODataReaderState.NestedProperty: | |||
// Property without value - do nothing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do nothing? need more details
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xuzhg What do you suggest we should do here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we 'd add at least:
- Why do we need to catch this reader state in this reading loop?
- Why do we do nothing in this reader state?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why didn't we need a case for this state before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xuzhg @habbes The block for the ODataReaderState.NestedProperty
should have been added when we upgraded the ODL dependency to the version that contained this change OData/odata.net#2786.
In OData/odata.net#2786, we introduced support for reading property without value. Before that, we would throw an exception if we came across a property without value. When we changed that, the ODataReaderState.NestedProperty
state gets toggled when we read such a property. It's just that after upgrading the ODL dependency, we didn't add any test to verify behaviour when a payload containing a property without value is deserialized. It's easy to verify my theory here. If you add the test in this PR to ASP.NET Core OData release-8.x
branch, the control flow will reach the default
block and the Debug.Assert
statement will fail. That's the reason we need the block that I introduced.
"\"Description\":\"Whole grain bread\"," + | ||
"\"PublishDate\":\"1997-07-01\"," + | ||
"\"[email protected]\":\"#Edm.DateTimeOffset\"," + // OData annotation - Property without value | ||
"\"[email protected]\":true," + // Custom annotation - Property without value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about the other type of properties with value? (Complex, enum)
Fixes #1307 by handling a property without value gracefully when deserializing OData payloads